aspect: add independent rectangular-cell oracle for planar aspect#2781
Merged
Conversation
) The planar tests only check that the four backends agree. They share the same _cpu/_gpu kernels, so a shared defect like the missing-cellsize bug (#2760) passes anyway. The QGIS fixture is a real reference but uses square cells, so it never exercises rectangular cells or checks that cell size is read from the coordinates. Add an analytic oracle: build a plane z = gx*X + gy*Y on a coordinate grid with no 'res' attr (so resolution comes from the x/y spacing) and compare aspect() against the closed-form Horn compass value instead of against another backend. - Square-cell oracle passes unconditionally on numpy, dask, cupy, dask+cupy. - Rectangular-cell oracle is xfail(strict=False) pending #2760; it flips to XPASS automatically once that fix lands, flagging the marker for removal.
brendancol
commented
Jun 1, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: aspect: add independent rectangular-cell oracle for planar aspect
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
_aspect_oraclehand-copies the compass conversion fromaspect._cpu(lines 74-81). That duplication is the whole point of an independent oracle, but it means the two can drift apart if the source convention ever changes. The square-cell tests re-check the oracle against the real function on every run, so this is fine as written. A one-line comment saying the conversion is duplicated on purpose would save the next reader some confusion. (xrspatial/tests/test_aspect.py:265)- The flat case
gx == gy == 0has a branch in_aspect_oraclebut no test feeds it, so the -1 path is dead code in this PR. Either drop the branch or add a(0.0, 0.0)parametrize case. A flat-surface oracle case is cheap and worth having. (xrspatial/tests/test_aspect.py:271)
Nits (optional improvements)
- In the cupy oracle tests, the inner
if backend == 'dask+cupy' and not has_dask_array(): pytest.skip(...)only matters on a machine with CuPy but no dask, which is rare. Harmless, just slightly defensive. (xrspatial/tests/test_aspect.py:325)
What looks good
- The oracle asserts against an analytic value, not another backend. That is the gap the issue called out.
- Rectangular cells with no
resattr force resolution to come from coordinate spacing, which is the exact path the missing-cellsize bug breaks. - The
xfail(strict=False)pointing at #2760 is the right call: green CI now, automatic XPASS when the fix lands. - Reuses
_to_numpyand the existing skip markers; coverage spans numpy, dask, cupy, dask+cupy.
Checklist
- Oracle matches the Horn compass convention in aspect._cpu
- Square-cell oracle passes on all available backends
- Rectangular-cell oracle xfails pending #2760
- Resolution is taken from coordinates (no res attr)
- Test-only change; aspect.py untouched
- flake8 clean on added lines
- Flat-surface (-1) oracle case not exercised
…2768) - Add (0.0, 0.0) to the square oracle params so the flat-surface -1 branch in _aspect_oracle is actually tested (it was dead code before). - Note in the docstring that the arctan2/compass conversion is duplicated from aspect._cpu on purpose, with the square tests guarding against drift.
brendancol
commented
Jun 1, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review
The two suggestions from the first pass are addressed in 12be968:
- The flat-surface case
(0.0, 0.0)is now in the square oracle params, so the -1 branch in_aspect_oracleis exercised. xrspatial returns -1 for a constant plane and the oracle agrees. - The docstring now says the arctan2/compass conversion is duplicated from
aspect._cpuon purpose, with the square tests guarding against drift.
The one remaining nit (the defensive has_dask_array() skip inside the cupy tests) is left as-is on purpose. It only matters on a CuPy-without-dask machine, and keeping it costs nothing while avoiding a confusing failure in that rare setup.
Oracle tests: 42 passed, 16 xfailed (numpy + dask; cupy paths skipped, no GPU here). flake8 clean on the added lines. No new findings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2768
What this does
The planar aspect tests only prove the four backends agree with each other. They share the same
_cpu/_gpukernels, so a defect common to every backend slips through. The missing-cellsize bug (#2760) is exactly that: planar aspect divides the Horn gradients by 8 but never bycellsize_x/cellsize_y, so it is wrong on non-square cells while every backend still matches. The QGIS fixture is a real external reference but uses square cells, so it never touches rectangular cells or checks that cell size comes from the coordinates.This adds an analytic oracle for planar aspect:
z = gx*X + gy*Yon a coordinate grid with noresattr, so resolution must be recovered from the x/y coordinate spacing.xfail(strict=False)referencing Planar aspect() ignores cell size; wrong for non-square cells #2760. It flips to XPASS automatically once that fix lands, which flags the marker for removal.This PR is test-only. It does not touch
aspect.py; the source fix is #2760's scope.Backend coverage
numpy, dask+numpy, cupy, dask+cupy. The cupy/dask+cupy oracle variants are gated behind the existing CUDA+CuPy skip marker.
Test plan
pytest xrspatial/tests/test_aspect.py-- 165 passed, 16 xfailed (numpy + dask; cupy paths skipped, no GPU here)When #2760 merges, the rectangular oracle should start XPASSing; remove the
xfailmarkers at that point.